Add config inheritance for personality field #169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wjt
force-pushed
the
T34731
branch
3 times, most recently
from
c569149 to
1c40da3
Compare
dbnicholson
reviewed
Nov 22, 2024
dbnicholson
reviewed
Nov 22, 2024
| for value in self._get_attr_values(attr): | ||
| path = os.path.join(dir_path, attr, value + '.ini') | ||
| namespace = dir_ns + '_'.join((attr, value)) | ||
| config_files.append((path, namespace)) |
There was a problem hiding this comment.
This appears to be a much nicer version of my idea for the last time this came up. See 3b3414c.
|
I'm all for this and make a quick attempt at this a while back that I never finished. |
|
It turns out the hard parts are:
|
Yeah, that is probably very annoying. I don't envy you...
It is deliberate. Hmm, I think I have a solution for the sorted merge... |
See #170. |
This was referenced Nov 22, 2024
It is often the case that we want to build an image which just applies a few minor changes to an existing personality. For instance, the apps we want for most Spanish images are the same, but we have a few country-specific applications, and we often wish to build a sub-variant for a partner or region of a particular country. Currently we need to copy the full list of apps & settings from our es.ini personality config into (for instance) es_MX.ini; and then again from es_MX.ini into (e.g.) es_MX_aguascalientes.ini to add some region-specific apps or resources Implement a form of inheritance where (e.g.) building the es_MX personality reads es.ini and es_MX.ini, in that order; and (e.g.) building the es_MX_aguascalientes personality reads es.ini, es_MX.ini and es_MX_aguascalientes.ini, again in that order. This matches the convention that we have of naming personalities for the locale, and then adding further underscore-separated suffixes for subvariants thereof. This is only implemented for the personality field. This is where we do most of our customisation and is the place where inheritance would be most useful. https://phabricator.endlessm.com/T34731
wjt
changed the title
Now that we have inheritance in personality configs, it becomes useful to be able to say that (for example) es_MX should add 1 specific channel to the selection inherited from es. Since commit 59980bc, values in merged fields keep the order in which they were first encountered. As a result, if (for example) the es personality specifies channels B, D, C (in that order) and es_MX adds channel A, then the resulting order is B, D, C, A. The order is significant: it is the order that channels are shown in the Kolibri Learn tab. So in this case it is not possible to move channel A to the top of the list without overriding the whole list of channels (which is possible but loses the benefit of this change). In practice, I only found two cases where this happens; in one case I hardcoded the previous order, and in the other case I just accepted the slight change. https://phabricator.endlessm.com/T34731
For projects in Haiti we have a (private) fr_HT personality. Checking our metrics, the majority of fr activations are in metropolitan France (the hexagonal bit of France that is just across the channel from the UK, rather than overseas regions).
dbnicholson
approved these changes
Nov 26, 2024
| ini_name = (value1 + '-' + value2 + '.ini') | ||
| path = os.path.join(dir_path, attr1 + '-' + attr2, ini_name) | ||
| namespace = (dir_ns + | ||
| '_'.join((attr1, attr2, value1, value2))) |
There was a problem hiding this comment.
Ugh, string joins with +. Older Dan is ashamed of younger Dan.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is often the case that we want to build an image which just applies a
few minor changes to an existing personality. For instance, the apps we
want for most Spanish images are the same, but we have a few
country-specific applications, and we often wish to build a sub-variant
for a partner or region of a particular country.
Currently we need to copy the full list of apps & settings from our
es.ini personality config into (for instance) es_MX.ini; and then again
from es_MX.ini into (e.g.) es_MX_aguascalientes.ini to add some
region-specific apps or resources
Implement a form of inheritance where (e.g.) building the es_MX
personality reads es.ini and es_MX.ini, in that order; and (e.g.)
building the es_MX_aguascalientes personality reads es.ini, es_MX.ini
and es_MX_aguascalientes.ini, again in that order. This matches the
convention that we have of naming personalities for the locale, and then
adding further underscore-separated suffixes for subvariants thereof.
This is only implemented for the personality field. This is where we do
most of our customisation and is the place where inheritance would be
most useful.
https://phabricator.endlessm.com/T34731